1204983 - Allow about: pages to load remote content

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Marcos Caceres [:marcosc]]

It would be nice if about:whatever could load a remote site, and not inherit the system principle.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

You mean redirect to a remote site?  Or have an iframe with a remote site?

It seems dangerous to leave the location bar at about:whatever, but be loading 3rd party content in the window.

Comment 2

[Boris Zbarsky [:bzbarsky]]

> It would be nice if about:whatever could load a remote site, and not inherit the system
> principle.

This is quite possible.  It would get a principal for about:whatever, of course, not the remote site it's loading, but nothing forces it into the system principal.

Comment 3

[Bobby Holley (:bholley)]

(In reply to Ben Kelly [:bkelly] from comment #1)
> It seems dangerous to leave the location bar at about:whatever, but be
> loading 3rd party content in the window.

The third-part content would be served from a Mozilla CDN.

Boris (and Gijs): Marcos and Olivier want to remote the about:newtab page and just make it forward to a regular webpage, rather than the chrome:// URI nastiness that we have now. This seems sensible to me, and it seems like we should be able to set it up so that this about: URI inherits the principal of whatever it redirects to. This maybe the case already, but I'm less familiar with about:// than I am with resource://.

So two questions:
(1) What is the current behavior of about: in this respect? Is there any machinery that we'd need to tweak to make this work?
(2) Are there other problems your see with this approach? Are there better solutions?

Comment 4

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #2)
> This is quite possible.  It would get a principal for about:whatever, of
> course, not the remote site it's loading, but nothing forces it into the
> system principal.

Hm, is there a way we could give it the principal of the remote content? Or maybe it doesn't really matter, in fact?

Comment 5

[Bobby Holley (:bholley)]

(I'm just generally skeptical of codebase principals for things like about:, since we occasionally have code that checks the URI principal and does weird stuff in those corner cases)

Comment 6

[Marcos Caceres [:marcosc]]

(In reply to Ben Kelly [:bkelly] from comment #1)
> You mean redirect to a remote site?

Kinda... not quite redirec, but "be" the remote site. I.e., load the content. 

>  Or have an iframe with a remote site?

We are doing this today. We literally have:

  <iframe id="remotedoc" src="https://mozilla.github.io/remote-newtab/"/>

> It seems dangerous to leave the location bar at about:whatever, but be
> loading 3rd party content in the window.

Hmm... it's what we want tho :( We are trying to find a way of making that secure.

Comment 7

[Boris Zbarsky [:bzbarsky]]

> Hm, is there a way we could give it the principal of the remote content?

We've generally avoided having the principal not matching what's shown in the url bar, so there is no simple way to do it right now.  We could add something, of course; it wouldn't be technically very hard.

> (1) What is the current behavior of about: in this respect?

There are two built-in behaviors, controlled by the URI_SAFE_FOR_UNTRUSTED_CONTENT about handler flag.  If that flag is set, the URI can be linked to by random web pages, but we null out the owner on the channel in the about protocol handler.  Then it gets a principal in the normal way, which typically means a codebase principal for the originalURI of the channel, since the channel is typically not redirected.

If that flag is not set, we disallow linking to the URI and don't SetOwner() on the channel.  Since the channel is typically chrome:// it has a system principal owner from the chrome protocol handler and gets the system principal.

> Is there any machinery that we'd need to tweak to make this work?

A question about desired behavior.  Are you looking to have the URL bar (document.location, whatever) show about:newtab but have the principal be http://whatever, or do we want document.location (and the docshell's URI, etc) to also be http://whatever?  The machinery that needs tweaking depends on the answer to this question.

> (2) Are there other problems your see with this approach?

I don't know that I understand the approach well enough to comment, though I do wonder whether the situation when the browser is opened without a network connection was considered.

Comment 8

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #7)
> We've generally avoided having the principal not matching what's shown in
> the url bar

about:newtab is a special case where we don't show anything in the URL bar at all, so I'm not sure this point matters much.

> , so there is no simple way to do it right now.  We could add
> something, of course; it wouldn't be technically very hard.

Ok. We _could_ also make the remote content run with an about:newtab codebase principal, but I think that's probably a bad idea.

Maybe the solution here is to use a proper redirect? The downside of this is that it would break any code that checks to see if the URI is about:newtab in order to make decisions (i.e. artificially blanking out the URL bar), but we may not want to design around that.

> > (1) What is the current behavior of about: in this respect?
> 
> There are two built-in behaviors, controlled by the
> URI_SAFE_FOR_UNTRUSTED_CONTENT about handler flag.  If that flag is set, the
> URI can be linked to by random web pages, but we null out the owner on the
> channel in the about protocol handler.  Then it gets a principal in the
> normal way, which typically means a codebase principal for the originalURI
> of the channel, since the channel is typically not redirected.

That's very helpful to know. If we want about:newtab to forward to mozillafoocdn.com/newtab, the originalURI would be about:newtab, right?

> If that flag is not set, we disallow linking to the URI and don't SetOwner()
> on the channel.  Since the channel is typically chrome:// it has a system
> principal owner from the chrome protocol handler and gets the system
> principal.

Yeah, I think that's what happens with about:newtab right now.

> A question about desired behavior.  Are you looking to have the URL bar
> (document.location, whatever) show about:newtab but have the principal be
> http://whatever, or do we want document.location (and the docshell's URI,
> etc) to also be http://whatever?  The machinery that needs tweaking depends
> on the answer to this question.

I think the docshell URI should be the URI of the remote content.

> I don't know that I understand the approach well enough to comment, though I
> do wonder whether the situation when the browser is opened without a network
> connection was considered.

Yes. The plan is to use a ServiceWorker. For the first-run case, they can either pre-prime the SW somehow, or just redirect to a local resource:// URI version if the load fails (using the same machinery that currently shows the network problem page).

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Marcos Caceres [:marcosc] from comment #6)
> >  Or have an iframe with a remote site?
> 
> We are doing this today. We literally have:
> 
>   <iframe id="remotedoc" src="https://mozilla.github.io/remote-newtab/"/>

This is probably a stupid question, but is there a reason not to just have a single iframe for the content containing the remote site?  Then the about:newtab window could remain as the system principal.  Right?

Comment 10

[Boris Zbarsky [:bzbarsky]]

> about:newtab is a special case where we don't show anything in the URL bar at all

Sure.  s/url bar/document.location/ since they're supposed to match, modulo some weirdness.

> If we want about:newtab to forward to mozillafoocdn.com/newtab, the originalURI would
> be about:newtab, right?

Yes.  I think we can have the protocol handler set originalURI, but docshell may override that...  I'd need o check.

> I think the docshell URI should be the URI of the remote content.

OK.  In that case, simply setting the LOAD_REPLACE flag on the channel will make it be treated as the final URI for all purposes: docshell URI, document.location, principal, etc.  Just like an HTTP redirect.

Comment 11

[Bobby Holley (:bholley)]

(In reply to Ben Kelly [:bkelly] from comment #9)
> This is probably a stupid question, but is there a reason not to just have a
> single iframe for the content containing the remote site?  Then the
> about:newtab window could remain as the system principal.  Right?

Yes, that's exactly what I'm trying to avoid, because it puts browser code on the content-side of the content/chrome docshell boundary, which means that the browser-implemented window is reachable via window.top. I want to get rid of these cases, and load remote content like a normal web page.

(In reply to Boris Zbarsky [:bz] from comment #10)
> Sure.  s/url bar/document.location/ since they're supposed to match, modulo
> some weirdness.

Sure.
 
> > If we want about:newtab to forward to mozillafoocdn.com/newtab, the originalURI would
> > be about:newtab, right?
> 
> Yes.  I think we can have the protocol handler set originalURI, but docshell
> may override that...  I'd need o check.

If I understand correctly, originalURI only matters if the channel doesn't set an owner (which both chrome:// and http:// do), right?

> > I think the docshell URI should be the URI of the remote content.
> 
> OK.  In that case, simply setting the LOAD_REPLACE flag on the channel will
> make it be treated as the final URI for all purposes: docshell URI,
> document.location, principal, etc.  Just like an HTTP redirect.

Hm, can you explain to me why we need LOAD_REPLACE at all? I just tried sending about:newtab to http://mozilla.org in AboutRedirector.cpp, and it seems to do exactly what we're talking about here.

Comment 12

[Boris Zbarsky [:bzbarsky]]

> If I understand correctly, originalURI only matters if the channel doesn't set an owner
> (which both chrome:// and http:// do), right?

originalURI matters for determining document.location, unless LOAD_REPLACE is set.

The owner is relevant for determining the principal.  chrome:// sets an owner, typically.  http:// does not.  If there is no owner (and nothing in the loadinfo about principal inheritance), then the principal is a codebase principal for document.location.

> Hm, can you explain to me why we need LOAD_REPLACE at all?

Because if that's not used, the principal will be a codebase principal for document.location which will be based on the originalURI of the channel, which I expect to be about:newtab in this case, no?

Comment 13

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #12)
> The owner is relevant for determining the principal.  chrome:// sets an
> owner, typically.  http:// does not.  If there is no owner (and nothing in
> the loadinfo about principal inheritance), then the principal is a codebase
> principal for document.location.
> 
> > Hm, can you explain to me why we need LOAD_REPLACE at all?
> 
> Because if that's not used, the principal will be a codebase principal for
> document.location which will be based on the originalURI of the channel,
> which I expect to be about:newtab in this case, no?

This wasn't what I observed in my experiments, but I now realize  that the issue is that I'd set about:newtab to point to http://www.mozilla.org, which redirects to https://www.mozilla.org/en-US/ . So during the redirect, everything got set up properly. When I set it up to point to https://www.mozilla.org/en-US/ directly, I observe the conditions you describe.

I'll attach a PoC that gives us hacks this together (we'd need to do it more elegantly with a flags were we actually to land this).

If Olivier and Marcos can work around the issue of not having about:newtab as the URI on the docshell, does this feel like a good approach to you Boris?

Comment 14

[Bobby Holley (:bholley)]

Attachment: 0001-Bug-1204983-Hacky-PoC.patch

Marcos, would behavior along these lines suit your needs?

Comment 15

[Boris Zbarsky [:bzbarsky]]

Seems fine to me.

Fwiw, it should still be possible to detect about:newtab loads by examining the channel's originalURI as needed... but I agree that it's a bit annoying.

Comment 16

[Marcos Caceres [:marcosc]]

Comment on attachment 8661590 [details] [diff] [review]
0001-Bug-1204983-Hacky-PoC.patch

Yeah, something like that would probably work.

Comment 17

[Bobby Holley (:bholley)]

Ok. Whenever someone wants to write it, I'm happy to mentor the proper patch (which would use flags rather than strcmp). I'd suggest using the PoC for now to make sure you can get everything else working the way you want.

Comment 18

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: bug_1204983_remote_about_newtab.patch

Bobby, I suppose this is what you had in mind, right? As discussed on IRC it should be possible to modify that newtab URI, how would you like to have that accomplished?

Comment 19

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: bug_1204983_remote_about_newtab_tests.patch

A simple browser mochitest that verifies that document.location as well as document.nodePrincipal have the right uri/principal attached. Anything more we need/should test?

Comment 20

[Bobby Holley (:bholley)]

Comment on attachment 8668164 [details] [diff] [review]
bug_1204983_remote_about_newtab.patch

Review of attachment 8668164 [details] [diff] [review]:
-----------------------------------------------------------------

I think we should simultaneously tighten things down to MOZ_RELEASE_ASSERT that about: URIs that redirect to non-chrome:// protocols have ALLOW_EXTERNAL_RESOURCE_LOAD set. Looking at the existing AboutRedirector implementations, it seems like about:credits is the only other place we'd need to add it. Hard to do an automated test for that one, so please just check it manually.

::: browser/components/about/AboutRedirector.cpp
@@ +84,5 @@
>      nsIAboutModule::URI_SAFE_FOR_UNTRUSTED_CONTENT |
>      nsIAboutModule::URI_MUST_LOAD_IN_CHILD |
>      nsIAboutModule::ALLOW_SCRIPT |
>      nsIAboutModule::ENABLE_INDEXED_DB },
> +  // TODO: replace hardcoded URL - how and where should that be defined though?

I would assume a pref, but you should probably ask Marcos and Olivier.

Comment 21

[Bobby Holley (:bholley)]

Comment on attachment 8668165 [details] [diff] [review]
bug_1204983_remote_about_newtab_tests.patch

Review of attachment 8668165 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!

::: browser/base/content/test/newtab/browser_newtab_external_resource.js
@@ +7,5 @@
> + */
> +
> +var gTestBrowser = null;
> +
> +// TODO: we have to replace that hardcoded URI within AboutRedirector

A perfect place to set the pref, I think!

Comment 22

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

(In reply to Bobby Holley (:bholley) from comment #20)
> I would assume a pref, but you should probably ask Marcos and Olivier.

Thanks Bobby!
I agree, a pref which allows to set the target url of about:newtab sounds reasonable to me as well.

Marcos, I got two questions:
a) What should be the default for the pref?
b) There are tests within 'browser/base/content/test/newtab/' which we need to update. Do those need to be rewritten or are we fine by just letting the pref point to 'chrome://browser/content/newtab/newTab.xhtml' (the current default) at the beginning of each test?

Comment 23

[Olivier Yiptong [:oyiptong]]

a)

The fx-team suggested we use an object overridable only at runtime as such:
https://dxr.mozilla.org/mozilla-central/source/browser/modules/NewTabURL.jsm

We've implemented our own at:
https://github.com/mozilla/newtab-dev/blob/c43e9f4813aac0a04ae92331b428c1ef54f5b383/browser/components/newtab/RemoteNewTabLocation.jsm

It manifests itself especially on preload. If we detect that the URL has been overridden, we don't preload the page, out of privacy concerns.

https://dxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#1651

We'll want the default to be a meta url. Something like:

https://newtab.cdn.mozilla.net/v1/%CHANNEL%/%LOCALE%/index.html

b)

We will completely get rid of newtab in favor of a new browser component we're making.

Comment 24

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: bug_1204983_remote_about_newtab.patch

Hey Bobby, I chatted with Olivier over IRC and they are fine with us landing the plumbing and then they will take it from there. So providing a basic pref sounds fine to them.

As far as the patch, I think extending 'struct RedirEntry' by an additonal entry which allows to specify a pref makes the most sense. We have to be careful that we are not setting the LOAD_REPLACE flag in case we are loading chrome:// about:// or file:// hence I special cased those. All tests within newtab/ continue to work.

I also extended the entry for 'credits' to also use the newly added flag, but I don't quite understand how we can manually check where we would need that? At least we have a RELEASE_ASSERT which will definitely tell us if we need to set it for some other entry, right?

Comment 25

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: bug_1204983_remote_about_newtab_tests.patch

Cleaned up the test a little as well and use the pref now to make about:newtab point to an external resource.

Comment 26

[Bobby Holley (:bholley)]

Comment on attachment 8669189 [details] [diff] [review]
bug_1204983_remote_about_newtab_tests.patch

Review of attachment 8669189 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/test/newtab/browser_newtab_external_resource.js
@@ +17,5 @@
> +    SpecialPowers.setCharPref(ABOUT_NEWTAB_URL_PREF, originalNewTabUrl);
> +  });
> +
> +  // remember the original pref
> +  originalNewTabUrl = SpecialPowers.getCharPref(ABOUT_NEWTAB_URL_PREF);

Instead of doing this stuff, please use SpecialPowers.pushPrefEnv, which does all of this automatically.

@@ +31,5 @@
> +    browser.removeEventListener("load", onLoad, true);
> +    is(content.document.location, URI, "document.location should match the external resource");
> +    is(content.document.documentURI, URI, "document.documentURI should match the external resource");
> +    is(content.document.nodePrincipal.URI.spec, URI, "nodePrincipal should match the external resource");
> +  }, true);

Don't you need to do something to make sure that the test actually waits until this event listener fires before completing?

Comment 27

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

(In reply to Bobby Holley (:bholley) from comment #26)
> Don't you need to do something to make sure that the test actually waits
> until this event listener fires before completing?

That's what I thought too and that's why I had waitForExplicitFinish in my inital patch, but apparently the machinery of newtab/head.js fired up by addNewTabPageTab seems to take care of that. Works for other tests within test/newtab as well so I assume it's fine, maybe we should get additonal review by someone who wrote those tests to make sure we hook them up correctly.

Comment 28

[Bobby Holley (:bholley)]

Comment on attachment 8669188 [details] [diff] [review]
bug_1204983_remote_about_newtab.patch

Review of attachment 8669188 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/about/AboutRedirector.cpp
@@ +22,5 @@
>  
>  struct RedirEntry {
>    const char* id;
>    const char* url;
> +  const char* pref; // allows to overwrite the default url

Nit: "allows overwriting"

@@ +184,5 @@
> +      }
> +      else {
> +        // default to the url specified in the map
> +        url = NS_LITERAL_CSTRING(kRedirMap[i].url);
> +      }

Please use the more modern mozilla/Preferences::* API, which lets you pass in the default in a single call. This handles the bug here that we don't currently handle the situation where no pref is found.

::: netwerk/protocol/about/nsAboutProtocolHandler.cpp
@@ +28,5 @@
>  
>    return (flags & nsIAboutModule::URI_SAFE_FOR_UNTRUSTED_CONTENT) != 0;
>  }
>  
> +static bool IsAllowedToReplaceLoad(nsIAboutModule *aModule, nsIURI *aURI, nsIChannel *aChannel) {

I think this should be called URIAllowsExternalResource

@@ +35,5 @@
> +
> +  bool isAboutChromeFile =
> +    (NS_SUCCEEDED(channelURI->SchemeIs("about", &isAboutChromeFile)) && isAboutChromeFile) ||
> +    (NS_SUCCEEDED(channelURI->SchemeIs("chrome", &isAboutChromeFile)) && isAboutChromeFile) ||
> +    (NS_SUCCEEDED(channelURI->SchemeIs("file", &isAboutChromeFile)) && isAboutChromeFile);

Do you really need all of these? AFAICT all of the about: URIs redirect to chrome: URIs

@@ +41,5 @@
> +  // if we are loading any of chrome://, about://, file:// then there is nothing we have to
> +  // worry about, otherwise we have to make sure that ALLOW_EXTERNAL_RESOURCE_LOAD is present.
> +  if (isAboutChromeFile) {
> +    return false;
> +  }

I think this is backwards. We should first check for the flag. If the flag isn't there, we should MOZ_RELEASE_ASSERT the scheme, and then return false.

Comment 29

[Bobby Holley (:bholley)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #27)
> (In reply to Bobby Holley (:bholley) from comment #26)
> > Don't you need to do something to make sure that the test actually waits
> > until this event listener fires before completing?
> 
> That's what I thought too and that's why I had waitForExplicitFinish in my
> inital patch, but apparently the machinery of newtab/head.js fired up by
> addNewTabPageTab seems to take care of that. Works for other tests within
> test/newtab as well so I assume it's fine, maybe we should get additonal
> review by someone who wrote those tests to make sure we hook them up
> correctly.

I don't understand how that could possibly work, given that the event handler you add doesn't resolve a promise or anything. Can you get to the bottom of it so that we can be sure this isn't racey?

Comment 30

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

(In reply to Bobby Holley (:bholley) from comment #28)

Hey Bobby, thanks for the speedy response, let me try to clarify:

> Please use the more modern mozilla/Preferences::* API, which lets you pass
> in the default in a single call. This handles the bug here that we don't
> currently handle the situation where no pref is found.

Agreed, that would be nice, but when including "mozilla/Preferences.h" we are hitting:
> #error "This header is only usable from within libxul (MOZILLA_INTERNAL_API)."
see: http://mxr.mozilla.org/mozilla-central/source/modules/libpref/Preferences.h#10
I assume that's why all of components/* still uses the old fashioned way of using do_GetService(NS_PREFSERVICE_CONTRACTID).

> > +  bool isAboutChromeFile =
> > +    (NS_SUCCEEDED(channelURI->SchemeIs("about", &isAboutChromeFile)) && isAboutChromeFile) ||
> > +    (NS_SUCCEEDED(channelURI->SchemeIs("chrome", &isAboutChromeFile)) && isAboutChromeFile) ||
> > +    (NS_SUCCEEDED(channelURI->SchemeIs("file", &isAboutChromeFile)) && isAboutChromeFile);
> 
> Do you really need all of these? AFAICT all of the about: URIs redirect to
> chrome: URIs

All of my testing confirms that we definitely need 'about' because of 'about:blank' and also 'file', because what I have seen so far confirms that all chrome:// URIs resolve to file://. Presumably we can remove chrome://, at least all of my testing confirms it's not needed.

> @@ +41,5 @@
> > +  // if we are loading any of chrome://, about://, file:// then there is nothing we have to
> > +  // worry about, otherwise we have to make sure that ALLOW_EXTERNAL_RESOURCE_LOAD is present.
> > +  if (isAboutChromeFile) {
> > +    return false;
> > +  }
> 
> I think this is backwards. We should first check for the flag. If the flag
> isn't there, we should MOZ_RELEASE_ASSERT the scheme, and then return false.

I would prefer early returns for that function. If we would check for the flag first, then we would already return 'true' if the flag is set even if the about: URL maps to a chrome: URL. Because of that we would set the LOAD_REPLACE flag on the channel, which is *incorrect* as far as I know. I think we should only set the LOAD_REPLACE flag if we do indeed forward to an external page/protocol. Or am I missing something?

Comment 31

[Bobby Holley (:bholley)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #30)
> I assume that's why all of components/* still uses the old fashioned way of
> using do_GetService(NS_PREFSERVICE_CONTRACTID).

Hm, ok then.

> All of my testing confirms that we definitely need 'about' because of
> 'about:blank' and also 'file', because what I have seen so far confirms that
> all chrome:// URIs resolve to file://. Presumably we can remove chrome://,
> at least all of my testing confirms it's not needed.

Ah right, because we end resolving through the chrome:// protocol handler and to the underlying file:// URI by the time we get there. Hm.

I guess checking for about: and file: is probably fine.

> > @@ +41,5 @@
> > > +  // if we are loading any of chrome://, about://, file:// then there is nothing we have to
> > > +  // worry about, otherwise we have to make sure that ALLOW_EXTERNAL_RESOURCE_LOAD is present.
> > > +  if (isAboutChromeFile) {
> > > +    return false;
> > > +  }
> > 
> > I think this is backwards. We should first check for the flag. If the flag
> > isn't there, we should MOZ_RELEASE_ASSERT the scheme, and then return false.
> 
> I would prefer early returns for that function. If we would check for the
> flag first, then we would already return 'true' if the flag is set even if
> the about: URL maps to a chrome: URL. Because of that we would set the
> LOAD_REPLACE flag on the channel, which is *incorrect* as far as I know.

I don't think that case matters. Nobody should be setting LOAD_REPLACE on an about: URI that maps to chrome:, but if even if they did, it wouldn't have any effect because chrome:// URIs set an owner on the channel.

The case we're trying to avoid is the one where somebody points to an external resource but doesn't set LOAD_REPLACE (since then we'd end up with a codebase principal for the originalURI of the channel, i.e. the about: URI). We can ensure this with the proper flags, and the MOZ_RELEASE_ASSERT is just making sure that nobody screwed up the flags.

Does that make sense?

Comment 32

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

(In reply to Bobby Holley (:bholley) from comment #31)
> The case we're trying to avoid is the one where somebody points to an
> external resource but doesn't set LOAD_REPLACE (since then we'd end up with
> a codebase principal for the originalURI of the channel, i.e. the about:
> URI).

Agreed. If someone sets the pref so that about:newtab points to some https://whatever.com/foo we have to make sure the flag ALLOW_EXTERNAL_RESOURCE_LOAD is set and hence our machinery would apply the LOAD_REPLACE on the channel.

> We can ensure this with the proper flags, and the MOZ_RELEASE_ASSERT
> is just making sure that nobody screwed up the flags.

Agreed, that's what I am trying to do here as well. The only problem we are facing is that if about:newtab forwards to some chrome:// then we would still have the ALLOW_EXTERNAL_RESOURCE_LOAD on that entry in our map and would therefore apply LOAD_REPLACE. It's hard to debug but there must be some side effect we are not considering at the moment because all of the tests within newtab/* stop working if we apply LOAD_REPLACE to regular chrome:// loads.

Comment 33

[Bobby Holley (:bholley)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #32)
> Agreed, that's what I am trying to do here as well. The only problem we are
> facing is that if about:newtab forwards to some chrome:// then we would
> still have the ALLOW_EXTERNAL_RESOURCE_LOAD on that entry in our map and
> would therefore apply LOAD_REPLACE. It's hard to debug but there must be
> some side effect we are not considering at the moment because all of the
> tests within newtab/* stop working if we apply LOAD_REPLACE to regular
> chrome:// loads.

Oh I see - so you're trying to dynamically set this flag, rather than just asserting that it was set. So we basically need to support two modes possibly modes for the same about: URI, depending on whether somebody sets an internal or external resource in the pref.

One way to do that would be to check whether the channel has an owner, and only set LOAD_REPLACE if it doesn't. Does that work?

So maybe we want to do the following in nsAboutProtocolHandler:

* Check if the channel has an explicit

Comment 34

[Bobby Holley (:bholley)]

(Ignore the last part of that comment)

We also probably need to preserve the old behavior of about:credits, and not set LOAD_REPLACE there somehow.

Comment 35

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: bug_1204983_remote_about_newtab.patch

(In reply to Bobby Holley (:bholley) from comment #33)
> Oh I see - so you're trying to dynamically set this flag, rather than just
> asserting that it was set. So we basically need to support two modes
> possibly modes for the same about: URI, depending on whether somebody sets
> an internal or external resource in the pref.
> 
> One way to do that would be to check whether the channel has an owner, and
> only set LOAD_REPLACE if it doesn't. Does that work?

That would be one option and it seems to work (at least what i can tell from my local testing). In my opinion the most intuitive option would be to rename the function to 'UsesAndAllowsExternalURILoad' which returns false if the scheme is either about:// or file:// and returns only true if the flag ALLOW_EXTERNAL_RESOURCE_LOAD is set, but it's totally up to you. We could have the early return for the two local schemese and have a MOZ_RELEASE_ASSERT on the ALLOW_EXTERNAL_RESOURCE_LOAD flag.

Comment 36

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: bug_1204983_remote_about_newtab_tests.patch

(In reply to Bobby Holley (:bholley) from comment #26)
> Instead of doing this stuff, please use SpecialPowers.pushPrefEnv, which
> does all of this automatically.

Thanks for pointing that out. I copied another test within about/newtab as a stub and just filled in the places I needed.
 
> Don't you need to do something to make sure that the test actually waits
> until this event listener fires before completing?

To make sure the test is not racy we can wait for the onLoadRequest and then manually move on to the next test using TestRunner.next();

Comment 37

[Bobby Holley (:bholley)]

Comment on attachment 8670094 [details] [diff] [review]
bug_1204983_remote_about_newtab_tests.patch

Review of attachment 8670094 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/test/newtab/browser_newtab_external_resource.js
@@ +11,5 @@
> +
> +var browser = null;
> +
> +function runTests() {
> +  // set the pref for about:newtab to point to an exteranl resource

Nit: "external"

@@ +20,5 @@
> +  browser = gWindow.gBrowser.selectedBrowser;
> +  browser.contentWindow.location = "about:newtab";
> +
> +  browser.addEventListener("load", function onLoad() {
> +    browser.removeEventListener("load", onLoad, true);

Can you also test what happens when we use the default chrome:// about:newtab page? We should end up with system principal for that oen (at least for now)

Comment 38

[Bobby Holley (:bholley)]

Comment on attachment 8670088 [details] [diff] [review]
bug_1204983_remote_about_newtab.patch

Review of attachment 8670088 [details] [diff] [review]:
-----------------------------------------------------------------

So - I feel bad about dragging this out, but I think I'm not totally satisfied in what we've ended up with here.

Basically, the nsIAboutModule flag we're adding was supposed to map 1-1 with having LOAD_REPLACE on the channel. But because the target URI might be _either_ chrome:// or http://, and because LOAD_REPLACE apparently breaks for chrome:// URIs, the flag doesn't really help us with anything, aside from giving us something to make assertions about.

Moreover, I think this does the wrong thing for about:credits, since the LOAD_REPLACE will cause the actual http URI for about:credits to appear in the address bar after you type it (please confirm this if you have a build with this patch).

So I think we should simplify this a bit and keep this special snowflake inside AboutRedirector.cpp. Instead of having a pref, we can have an optional function pointer to generate both the URI and the flags to pass to NS_NewChannel. Thoughts?

Comment 39

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: bug_1204983_remote_about_newtab.patch

(In reply to Bobby Holley (:bholley) from comment #38)
> So I think we should simplify this a bit and keep this special snowflake
> inside AboutRedirector.cpp. Instead of having a pref, we can have an
> optional function pointer to generate both the URI and the flags to pass to
> NS_NewChannel. Thoughts?

Hey Bobby, I agree with you on the first part, we should move the special code handling into AboutRedirector. At the same time I think we should not over engineer the solution to the problem we are trying to solve here. I kind of like the idea with the pref which allows one to overwrite the default location for about:newtab. What if we just set the LOAD_REPLACE flag when calling NS_NewChannel() within AboutRedirector in case we are loading something else than chrome:? That approach would allow us to set the flag where needed, at the same time leave about:credits untouched, but still allowing us to assert within nsAboutProtocolHandler that the additional flag is set in case we are loading something other than chrome: !

What do you think? (Not sure if that was your suggestion all along modulo the function pointer).

Once we have agreed on an approach I will extend the test in the other patch accordingly.

Comment 40

[Bobby Holley (:bholley)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #39)
> Hey Bobby, I agree with you on the first part, we should move the special
> code handling into AboutRedirector. At the same time I think we should not
> over engineer the solution to the problem we are trying to solve here. I
> kind of like the idea with the pref which allows one to overwrite the
> default location for about:newtab. What if we just set the LOAD_REPLACE flag
> when calling NS_NewChannel() within AboutRedirector in case we are loading
> something else than chrome:? That approach would allow us to set the flag
> where needed, at the same time leave about:credits untouched

You mean because about:credits lives in nsAboutRedirector instead of AboutRedirector? That seems a bit fragile...

I think we should do what you suggest, but _also_ check against URI_IS_SAFE_FOR_UNTRUSTED_CONTENT and not add LOAD_REPLACE in that case.

, but still
> allowing us to assert within nsAboutProtocolHandler that the additional flag
> is set in case we are loading something other than chrome: !

Doing that in nsAboutProtocolHandler is annoying as you discovered because we've already resolved through chrome://, and because we need to handle the about:credits case, which basically does the opposite of what we want here. So I'd say we should just forget about the assertion.

Comment 41

[Bobby Holley (:bholley)]

Comment on attachment 8670617 [details] [diff] [review]
bug_1204983_remote_about_newtab.patch

Review of attachment 8670617 [details] [diff] [review]:
-----------------------------------------------------------------

Per above, I don't think that the aboutModule flag and release assert are really buying us much.

::: browser/components/about/AboutRedirector.cpp
@@ +198,5 @@
> +      // If the URI in the pref links to an external URI (other than chrome://),
> +      // then set the LOAD_REPLACE flag on the channel which forces the channel
> +      // owner to reflect the displayed URL rather then being the systemPrincipal.
> +      bool isChromeScheme =
> +        (NS_SUCCEEDED(tempURI->SchemeIs("chrome", &isChromeScheme)) && isChromeScheme);

The NS_SUCCEEDED thing here is fishy, because it's not clear that either choice for isChromeScheme is safe if the calls fails for some reason. I'd replace it with an NS_ENSURE_SUCCESS(rv, rv);

Comment 42

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: bug_1204983_remote_about_newtab.patch

Alright, so back to the most simple approach and keep all the logic within AboutRedirector :-)

Comment 43

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: bug_1204983_remote_about_newtab_tests.patch

Extended the test to also cover the default behavior where the nodePrincipal should match the systemPrincipal. Carrying over r+ from bholley.

Comment 44

[Bobby Holley (:bholley)]

Comment on attachment 8671103 [details] [diff] [review]
bug_1204983_remote_about_newtab.patch

Review of attachment 8671103 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with those. Let's have a quick sr from bz.

::: browser/components/about/AboutRedirector.cpp
@@ +183,5 @@
> +      if (strcmp(kRedirMap[i].pref, "") > 0) {
> +        rv = prefs->GetCharPref(kRedirMap[i].pref, getter_Copies(url));
> +        NS_ENSURE_SUCCESS(rv, rv);
> +      }
> +      else {

Instead of |else|, I think this should |url.IsEmpty()| so that we fall back to the hard-coded url as-appropriate.

@@ +185,5 @@
> +        NS_ENSURE_SUCCESS(rv, rv);
> +      }
> +      else {
> +        // default to the url specified in the map
> +        url = NS_LITERAL_CSTRING(kRedirMap[i].url);

This isn't really a literal string because the compiler doesn't know its length at compile-time. You want nsDependentCString here.

@@ +195,3 @@
>        NS_ENSURE_SUCCESS(rv, rv);
> +
> +      // If the URI in the pref links to an external URI (other than chrome://),

Grammatically, this implies that chrome:// is an external URI. I'd say (i.e. something other than chrome://). But see below.

@@ +197,5 @@
> +      // If the URI in the pref links to an external URI (other than chrome://),
> +      // then set the LOAD_REPLACE flag on the channel which forces the channel
> +      // owner to reflect the displayed URL rather then being the systemPrincipal.
> +      bool isChromeScheme;
> +      rv = tempURI->SchemeIs("chrome", &isChromeScheme);

I think we should also support resource:// here, which is probably a smarter choice in general for most of these about: URIs. So let's do that, and have |bool isChrome| and |bool isRes|, and |bool isExternal = !isChrome && !isRes|.

Comment 45

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8671103 [details] [diff] [review]
bug_1204983_remote_about_newtab.patch

>+      if (strcmp(kRedirMap[i].pref, "") > 0) {

Might be simpler to have:


  if (*kRedirMap[i].pref) {

>+        url = NS_LITERAL_CSTRING(kRedirMap[i].url);

It's not a literal.  I'm surprised this worked at all.  Need to either url.AssignASCII(kRedirMap[i].url) or something along those lines, no?

>+      rv = NS_NewURI(getter_AddRefs(tempURI), url.get());

Drop the .get().  It's not needed and just causes the callee to do more work.

But since this is supposed to be an sr....  This patch is not making sense to me in the grand scheme of things.  We just _removed_ a way to control what the default tab page is via prefs (see bug 1118285).  Now we're adding a new one?  Please at least coordinate with the people who removed the old one or something.

Comment 46

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #45)
> But since this is supposed to be an sr....  This patch is not making sense
> to me in the grand scheme of things.  We just _removed_ a way to control
> what the default tab page is via prefs (see bug 1118285).  Now we're adding
> a new one?  Please at least coordinate with the people who removed the old
> one or something.

/me giggles

Sounds like Content Services and Firefox need to work out what they want to do here. Good thing I flagged Boris!

Comment 47

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: bug_1204983_remote_about_newtab.patch

Addressed all the feedback from Bobby and Boris so the patch would be ready in case we want it at all :-)

(In reply to Bobby Holley (:bholley) from comment #46)
 > Sounds like Content Services and Firefox need to work out what they want to
> do here. Good thing I flagged Boris!

Yep, thanks for the input. Let's wait for now and see what happens.

Comment 48

[Marcos Caceres [:marcosc]]

(In reply to Bobby Holley (:bholley) from comment #46)
> (In reply to Boris Zbarsky [:bz] from comment #45)
> > But since this is supposed to be an sr....  This patch is not making sense
> > to me in the grand scheme of things.  We just _removed_ a way to control
> > what the default tab page is via prefs (see bug 1118285).  Now we're adding
> > a new one?  Please at least coordinate with the people who removed the old
> > one or something.
> 
> /me giggles
> 
> Sounds like Content Services and Firefox need to work out what they want to
> do here. Good thing I flagged Boris!

We (content services) don't want the URL to be configurable via a pref (the pref that we have right now goes from "about:newtab" to "about:remote-newtab"). However, we need to be able to override the URL for development purposes (e.g., to http://localhost:8000). 

We have a JSM that lets us do that, called "RemoteNewTabLocation.jsm". A plugin, however, could use RemoteNewTabLocation.jsm to override the URL.... maybe we should make it that "RemoteNewTabLocation" is limited to a small set of URLs.

Comment 49

[Olivier Yiptong [:oyiptong]]

We (Content Services) are making a remotely hosted newtab page. We currently need to redirect the newtab page to a chrome:// location and render our remote newtab page inside an iframe.

This patch will allow us to redirect the newtab page directly to the page and will allow us to cull some unneeded machinery.

The URL will be overridable, in the exact same way NewTabURL works.

As Marcos mentioned, it is still useful to be able to override the url. One of the reasons is for development purposes, another reason is to build automation for tests.

The newtab page's url is given by: https://dxr.mozilla.org/mozilla-central/source/browser/modules/NewTabURL.jsm

Comment 50

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: bug_1204983_remote_about_newtab.patch

Bobby, I suppose we should register an observer then, right? Here is an initial proof of concept patch, obviously this needs to be cleaned up and we can definitely be smarter about copying around those URL strings.

Marcos, Olivier, would that work for you?

Comment 51

[Bobby Holley (:bholley)]

Comment on attachment 8671674 [details] [diff] [review]
bug_1204983_remote_about_newtab.patch

Review of attachment 8671674 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/about/AboutRedirector.cpp
@@ +192,5 @@
>    return path;
>  }
>  
> +AboutRedirector::AboutRedirector()
> +  : mAboutNewTabObserver(new AboutNewTabObserver())

This _probably_ works, but only in the case where the AboutRedirector gets instantiated before we load the JSM, which is kind of a tricky dependency. Moreover, the current things that NewTabURL gives you is 'about:newtab', which is going to send you in an infinite loop.

It seems like there's a mismatch between what that JSM gives you (which is the value we expect to end up in document.location), and what we need here in C++ (which is the place that we redirect about:newtab to).

Here's what I would suggest:
(1) Replace the .JSM with an XPCOM service in simpleServices.js (this will save an extra global too).
(2) Implement logic in that service that correctly distinguishes the URI referent and the expected document.location, and provides getters for both (basically just store the URI referent and return 'about:newtab' if the scheme is something we won't use LOAD_REPLACE on).
(3) Nix the pref observer, and query that service directly from C++ here.

But before any more code gets written, I think the three of y'all should sit down and have an explicit discussion about exactly what needs to be done to satisfy everyone's requirements.

Comment 52

[Marcos Caceres [:marcosc]]

> But before any more code gets written, I think the three of y'all should sit down and have an explicit discussion about exactly what needs to be done to satisfy everyone's requirements.

Yeah, we are around all today :) Christoph, pinged you on IRC to arrange a time.

Comment 53

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: bug_1204983_remote_about_newtab.patch

Bobby, so basically what I did is that I converted NewTabURL.jsm into an XPCOM which is implemented in simpleServices.js. On purpose it's pretty much matching the functionality of NewTabURL.jsm, which actually should be fine for what we want. In the end we want to replace NewTabURL.jsm with it, right?

It's working and pretty much ready for review. Just to make sure, we only want to use LOAD_REPLACE for schemes other than chrome:// and resource:// or are there other schemes we have to take into account?

The next patch then would include updating/replacing/removing all of the usage of NewTabUrl.jsm which is quite a lot actually [1].

Florian, Olivier, Marcos, we all agree on that, right? Just making sure that we are not missing anything here.

[1] http://mxr.mozilla.org/mozilla-central/search?string=NewTabURL.&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

Comment 54

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: bug_1204983_remote_about_newtab_tests.patch

Also updated the tests to use nsIAboutNewTabService rather than NewTabURL.jsm. Everything else remain unchanged. Carrying over r+ from bholley.

Comment 55

[Marcos Caceres [:marcosc]]

Agreed. Thanks again for doing this so quickly Christoph!

Comment 56

[Florian Quèze [:florian]]

Comment on attachment 8672200 [details] [diff] [review]
bug_1204983_remote_about_newtab.patch

Review of attachment 8672200 [details] [diff] [review]:
-----------------------------------------------------------------

Overall, I have no problem with NewTabUrl.jsm becoming an XPCOM component instead. But, I see a few issues in the current patch.

Unrelated to the current patch, the discussion in this bug makes me a bit uncomfortable, as I don't see any mention of how the user will be aware of what's going on. If we are loading a remote page, in my opinion this needs to be visible to the user. There are privacy implications to firing a network requests whenever the user opens a new tab. I would assume/hope we won't request data from the network for each new tab, but I also don't know how that fact can be surfaced to privacy-sensitive users (who are arguably only a small subset of users).

::: browser/components/nsIAboutNewTabService.idl
@@ +14,5 @@
> +{
> +  /**
> +   * Forward about:newtab to point to aNewTabURL
> +   */
> +   void overrideNewTabURL(in ACString aNewTabURL);

nit: this should be 2-space indent, not 3.

Having this settable through a C++-accessible API is slightly unfortunate, as one of the reasons for the changes in bug 1118285 was that some malware was abusing the pref with a C++ pref observer that was changing back the value immediately when the user reverted it in about:config. I'm willing to say that this doesn't matter unless it becomes abused again (in which case we could change in a future bug this setter to be only accessible from JS).

@@ +20,5 @@
> +   /**
> +    * Returns "about:newtab" if not overridden, otherwise
> +    * a string represenation of the new URL.
> +    */
> +   ACString getNewTabURL();

I wonder if we should simplify this API and replace overrideNewTabURL and getNewTabURL with just a read/write 'newTabURL' attribute.

Is ACString the correct type? I'm not sure how IDN urls work, but I wonder if we need UTF8 here (ie. AUTF8String).

@@ +26,5 @@
> +   /**
> +    * Returns true if the default of "about:newtab" got
> +    * overridden.
> +    */
> +   bool getOverridden();

This should be:
  readonly attribute boolean overridden;

::: toolkit/components/utils/simpleServices.js
@@ +141,5 @@
> +}
> +
> +AboutNewTabService.prototype = {
> +  classID: Components.ID("{97eea4bb-db50-4ae0-9147-1e5ed55b4ed5}"),
> +  QueryInterface: XPCOMUtils.generateQI([Ci.nsIAboutNewTabService]),

This API is defined in browser/, and is browser-specific. The implementation of this component shouldn't be in the toolkit/ folder.

@@ +147,5 @@
> +  overrideNewTabURL(aNewTabURL) {
> +    this.newtabURL = aNewTabURL;
> +    this.overridden = true;
> +    // do we want the observerservice here?
> +    // Services.obs.notifyObservers(null, "newtab-url-changed", this._url);

Yes, we do. Add-on authors insisted we need this notification, so that their well-meaning add-on can detect that another add-on has already customized the new tab URL, and that they shouldn't fight each other to override the URL.

@@ +163,5 @@
> +    this.newtabURL = "about:newtab";
> +    this.overridden = false;
> +    // do we want the observerservice here?
> +    // Services.obs.notifyObservers(null, "newtab-url-changed", this._url);
> +   }

nit: indent.

Comment 57

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

(In reply to Florian Quèze [:florian] [:flo] from comment #56)
> Overall, I have no problem with NewTabUrl.jsm becoming an XPCOM component
> instead. But, I see a few issues in the current patch.

Great, I am happy that we can all agree on that approach. As far as the patch goes, we can definitely convert some functions into attributes and make them readonly and what not. Let's also wait for bobby's input and then we can finalize it.
 
> Unrelated to the current patch, the discussion in this bug makes me a bit
> uncomfortable, as I don't see any mention of how the user will be aware of
> what's going on. If we are loading a remote page, in my opinion this needs
> to be visible to the user. There are privacy implications to firing a
> network requests whenever the user opens a new tab. I would assume/hope we
> won't request data from the network for each new tab, but I also don't know
> how that fact can be surfaced to privacy-sensitive users (who are arguably
> only a small subset of users).

That is a good point. As of now I am not aware of any existing mechanism we could use to make users aware of loading a remote page in such cases. It's definitely worth discussing though. On the other hand we are just providing the plumbing in this bug, which will allow the loading of remote pages. Currently, this ground work will mostly be used for testing as far as I know. Nevertheless, we should definitely keep an eye on that issue, because I know privacy sensitive people would like to know what's going on behind the scenes when loading a new tab.

Comment 58

[Bobby Holley (:bholley)]

(In reply to Florian Quèze [:florian] [:flo] from comment #56)
> Having this settable through a C++-accessible API is slightly unfortunate,
> as one of the reasons for the changes in bug 1118285 was that some malware
> was abusing the pref with a C++ pref observer that was changing back the
> value immediately when the user reverted it in about:config. I'm willing to
> say that this doesn't matter unless it becomes abused again (in which case
> we could change in a future bug this setter to be only accessible from JS).

If somebody is hooking into Gecko with arbitrary C++, they can very easily execute arbitrary JS, so I don't think you much at all by restricting this API to JS.

> This API is defined in browser/, and is browser-specific. The implementation
> of this component shouldn't be in the toolkit/ folder.

Fair enough, but please make a similar simpleServices in browser, so that we can avoid the proliferation of new globals for super-simple APIs like this one.

Comment 59

[Bobby Holley (:bholley)]

Comment on attachment 8672200 [details] [diff] [review]
bug_1204983_remote_about_newtab.patch

Review of attachment 8672200 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/about/AboutRedirector.cpp
@@ +160,5 @@
>  
> +AboutRedirector::AboutRedirector()
> +{
> +  mAboutNewTabService = do_GetService("@mozilla.org/addons/aboutnewtab-service;1");
> +  // do we want an assertion or do we need a nullcheck somewhere else in the code?

MOZ_ASSERTing it is fine.

Comment 60

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: bug_1204983_remote_about_newtab.patch

Hey Bobby, one thing I am not sure at this point, are we sure we want to set the LOAD_REPLACE flag on everything other than chrome:// and resource://? Probably it is, just making sure we are not missing something here.

Once we are fine with the patch (all attributes, functions within the *.idl) I am going to upload an updated test. Probably we should also include a test for the observer service.

Also, once you are fine with the patch I am going to ask Florian for a super review to make sure we are not missing anything.

Comment 61

[Bobby Holley (:bholley)]

Comment on attachment 8673176 [details] [diff] [review]
bug_1204983_remote_about_newtab.patch

Review of attachment 8673176 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #60)
> Hey Bobby, one thing I am not sure at this point, are we sure we want to set
> the LOAD_REPLACE flag on everything other than chrome:// and resource://?
> Probably it is, just making sure we are not missing something here.

Now that you mention it, I think we should also condition it on SAFE_FOR_UNTRUSTED_CONTENT not being set, so that if about:credits were defined in this redirector (which it isn't), we'd do the right thing.

Once you fix that (and the rest of the comments), please ask bz to sr the flag handling in AboutRedirector.

::: browser/components/about/AboutRedirector.cpp
@@ +162,5 @@
>  }
>  
> +AboutRedirector::AboutRedirector()
> +{
> +  mAboutNewTabService = do_GetService("@mozilla.org/browser/aboutnewtab-service;1");

Leak-wise, I think it's probably safer to ditch this cache and just do do_GetService directly in the code below where we need it (only in the case that we match "newtab", of course).

::: browser/components/nsIAboutNewTabService.idl
@@ +25,5 @@
> +  readonly attribute bool overridden;
> +
> +  /**
> +   * Resets "about:newtab" to the default and also resets the
> +   * overridden flag so taht getOverridden() returns false again.

Nit: 'that'.

::: browser/components/utils/simpleBrowserServices.js
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/*
> + * Dumping ground for simple services withinb browser/ for which the isolation

'within'

Comment 62

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: bug_1204983_remote_about_newtab.patch

(In reply to Bobby Holley (:bholley) from comment #61)
> Now that you mention it, I think we should also condition it on
> SAFE_FOR_UNTRUSTED_CONTENT not being set, so that if about:credits were
> defined in this redirector (which it isn't), we'd do the right thing.
> 
> Once you fix that (and the rest of the comments), please ask bz to sr the
> flag handling in AboutRedirector.

I updated the patch and fixed your nits, but I do think that we should file a separate bug in case we really want to address the handling of SAFE_FOR_UNTRUSTED_CONTENT. It doesn't seem to be related to this bug in my opinion. If you insist, I can obviously update that as well, but I haven't touched those bits within the updated patch. Carrying over r+ from bholly.

Comment 63

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Comment on attachment 8673285 [details] [diff] [review]
bug_1204983_remote_about_newtab.patch

Boris, can you take a final look at this patch? Please also see comment 62.

Comment 64

[Bobby Holley (:bholley)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #62)
> I updated the patch and fixed your nits, but I do think that we should file
> a separate bug in case we really want to address the handling of
> SAFE_FOR_UNTRUSTED_CONTENT. It doesn't seem to be related to this bug in my
> opinion.

I disagree. This patch effectively adds support for a third mode in addition to the two listed in comment 7. We need to make sure that this third mode does not disrupt the intended behavior of those first two modes. With the patch as it stands it will disrupt that behavior for use-cases like about:credits, which currently happens to be in nsAboutRedirector.cpp instead of AboutRedirector.cpp. It could arguably be moved though, and we shouldn't set up the machinery here so that moving it would create a subtle-but-important behavior change.

All you need to do is to check the nsIAboutModule flags for URI_IS_SAFE_FOR_UNTRUSTED_CONTENT, and make LOAD_REPLACE conditional on isChromeOrResource || isSafeForUntrustedContent.

Comment 65

[Bobby Holley (:bholley)]

(or rather, the inverse of that condition)

Comment 66

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

(In reply to Bobby Holley (:bholley) from comment #64)
> (In reply to Christoph Kerschbaumer [:ckerschb] from comment #62)
> > I updated the patch and fixed your nits, but I do think that we should file
> > a separate bug in case we really want to address the handling of
> > SAFE_FOR_UNTRUSTED_CONTENT. It doesn't seem to be related to this bug in my
> > opinion.
> 
> I disagree. This patch effectively adds support for a third mode in addition
> to the two listed in comment 7. We need to make sure that this third mode
> does not disrupt the intended behavior of those first two modes. With the
> patch as it stands it will disrupt that behavior for use-cases like
> about:credits, which currently happens to be in nsAboutRedirector.cpp
> instead of AboutRedirector.cpp. It could arguably be moved though, and we
> shouldn't set up the machinery here so that moving it would create a
> subtle-but-important behavior change.
> 
> All you need to do is to check the nsIAboutModule flags for
> URI_IS_SAFE_FOR_UNTRUSTED_CONTENT, and make LOAD_REPLACE conditional on
> isChromeOrResource || isSafeForUntrustedContent.

I am not sure I follow your thinking here. So, if the scheme is something else than chrome:// or resource:// then the current setup would use LOAD_REPLACE which would use a codebasePrincipal as the owner instead of the systemPrincipal, right? This would happen regardless of whether SAFE_FOR_UNTRUSTED_CONTENT is set or not.

*If* the scheme is chrome:// and the flag URI_IS_SAFE_FOR_UNTRUSTED_CONTENT is set, then we should still *not* use LOAD_REPLACE, but rather LOAD_NORMAL, which is what we do at the moment, no?

But maybe I am missing something.

Comment 67

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: bug_1204983_remote_about_newtab_deprecate_newtaburl.patch

Marcos, within this patch we are going to completely replace NewTabURL.jsm with aboutNewTabService.js. I assume we can remove the pref 'browser.newtabpage.remote' [1] completely, right? Or do we still need that for any reason?

[1] http://mxr.mozilla.org/mozilla-central/search?string=browser.newtabpage.remote

Comment 68

[Bobby Holley (:bholley)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #66)
> I am not sure I follow your thinking here. So, if the scheme is something
> else than chrome:// or resource:// then the current setup

By 'current setup' you mean the patch as-posted, right?

> would use
> LOAD_REPLACE which would use a codebasePrincipal as the owner instead of the
> systemPrincipal, right? This would happen regardless of whether
> SAFE_FOR_UNTRUSTED_CONTENT is set or not.

Sort of - see below.

> *If* the scheme is chrome:// and the flag URI_IS_SAFE_FOR_UNTRUSTED_CONTENT
> is set, then we should still *not* use LOAD_REPLACE, but rather LOAD_NORMAL,
> which is what we do at the moment, no?

Correct.

The case you're missing is when the scheme is not chrome:// or resource:// (something like http://) and when URI_IS_SAFE_FOR_UNTRUSTED_CONTENT is set (which is what about:credits does). In the current world, about:credits gets a codebase principal for about:credits. With your patch, we'd start passing LOAD_REPLACE, which would give us a codebase principal for http://www.mozilla.org/credits/, which is different behavior.

Funny thing though, it looks like mozilla.org recently started doing http -> https redirection, so about:credits hits a redirect, which sets LOAD_REPLACE on the channel anyway, which means that about:credits now lands you with |https://www.mozilla.org/credits/| in the URL bar rather than |about:credits|.

So it's sort of a moot point I guess. I was pushing back against the behavior change implicit in this patch because it was an unrelated (observable) behavior change. But it appears not to be in practice, and I think the new behavior is preferable (since it's always better to show the user the correct location of remote content).

So. I think my new recommendation is to do the following:
* Keep the LOAD_REPLACE behavior as-is in nsAboutRedirector.
* Add the same behavior to AboutRedirector.
* Fix about:credits to point to https instead of http, and verify that you still get the remote address in the URL bar when you type about:credits.

Comment 69

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

(In reply to Bobby Holley (:bholley) from comment #68)
> By 'current setup' you mean the patch as-posted, right?
Yes, the patch that is currently attached to the bug.

> The case you're missing is when the scheme is not chrome:// or resource://
> (something like http://) and when URI_IS_SAFE_FOR_UNTRUSTED_CONTENT is set
> (which is what about:credits does). In the current world, about:credits gets
> a codebase principal for about:credits. With your patch, we'd start passing
> LOAD_REPLACE, which would give us a codebase principal for
> http://www.mozilla.org/credits/, which is different behavior.

Ah, I see what you are saying. 'about:credits' was about to be displayed in the URL bar but the principal used would have been http(s)://www.mozilla.org/credits/. That makes indeed a difference in behavior.

> So. I think my new recommendation is to do the following:
> * Keep the LOAD_REPLACE behavior as-is in nsAboutRedirector.
> * Add the same behavior to AboutRedirector.
> * Fix about:credits to point to https instead of http, and verify that you
> still get the remote address in the URL bar when you type about:credits.

Thanks, yes, that sounds reasonable to me as well.

Comment 70

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: bug_1204983_remote_about_newtab.patch

(In reply to Bobby Holley (:bholley) from comment #68)
> So. I think my new recommendation is to do the following:
> * Keep the LOAD_REPLACE behavior as-is in nsAboutRedirector.
> * Add the same behavior to AboutRedirector.

Or the other way round :-) Keep the behavior in AboutRedirector and add the same behavior to nsAboutRedirector. Anyway, here we go. That should do it.

Comment 71

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Comment on attachment 8673413 [details] [diff] [review]
bug_1204983_remote_about_newtab.patch

Boris, we still want your input though. Thanks!

Comment 72

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8673413 [details] [diff] [review]
bug_1204983_remote_about_newtab.patch

>+      if (!strcmp(path.get(), "newtab")) {

Is there a reason that's not:

  if (path.EqualsLiteral("newtab")) {

?

>+      // If the URI in the pref links to an external URI (i.e. something

There is no pref.  Please fix this comment to reflect what the code is actually doing.

>+      rv = tempURI->SchemeIs("chrome", &isChrome);
>+      rv = tempURI->SchemeIs("resource", &isRes);

Is there a reason we're hardcoding schemes instead of using the URI_IS_UI_RESOURCE protocol flag?  This use case seems like exactly what it's designed for, as far as I can tell.

>+                                 isExternal
>+                                 ? nsIChannel::LOAD_REPLACE
>+                                 : nsIRequest::LOAD_NORMAL);

I'd prefer we put the flag calculation in a separate line before the NS_NewChannelInternal call, or at least indent the second/third lines here to make it clearer that it's all one argument.

Same comments apply to nsAboutRedirector.cpp.

r=me with those fixed.

Comment 73

[Florian Quèze [:florian]]

Comment on attachment 8673413 [details] [diff] [review]
bug_1204983_remote_about_newtab.patch

Review of attachment 8673413 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/moz.build
@@ +23,5 @@
>      'shell',
>      'selfsupport',
>      'tabview',
>      'uitour',
> +    'utils',

I don't think we need an 'utils' folder here. There are already idl and js files directly in components/

::: browser/components/nsIAboutNewTabService.idl
@@ +25,5 @@
> +  readonly attribute bool overridden;
> +
> +  /**
> +   * Resets "about:newtab" to the default and also resets the
> +   * overridden flag so that getOverridden() returns false again.

simplify the end of the sentence to: "also resets the overridden attribute to false."

::: browser/components/utils/simpleBrowserServices.js
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/*
> + * Dumping ground for simple services within browser/ for which the isolation

I don't think we need this new file. nsBrowserGlue.js seems a reasonable place to add this new tab service.

@@ +19,5 @@
> +Cu.import('resource://gre/modules/Services.jsm');
> +
> +function AboutNewTabService()
> +{
> +  this.newtabURL_ = "about:newtab";

this._newTabURL (and this._overridden)

::: browser/components/utils/utils.manifest
@@ +1,2 @@
> +component {97eea4bb-db50-4ae0-9147-1e5ed55b4ed5} simpleBrowserServices.js
> +contract @mozilla.org/browser/aboutnewtab-service;1 {97eea4bb-db50-4ae0-9147-1e5ed55b4ed5}

This should go in BrowserComponents.manifest

Comment 74

[Florian Quèze [:florian]]

Comment on attachment 8673390 [details] [diff] [review]
bug_1204983_remote_about_newtab_deprecate_newtaburl.patch

Review of attachment 8673390 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/utilityOverlay.js
@@ +9,5 @@
>  Components.utils.import("resource://gre/modules/PrivateBrowsingUtils.jsm");
>  Components.utils.import("resource:///modules/RecentWindow.jsm");
>  
> +let aboutNewTabService = Components.classes["@mozilla.org/browser/aboutnewtab-service;1"]
> +                                   .getService(Components.interfaces.nsIAboutNewTabService);

Use XPCOMUtils.defineLazyServiceGetter here.

This comment applies to all the places where you replaced an existing lazy module getter.

The getter being lazy doesn't matter for test code.

::: browser/components/newtab/NewTabURL.jsm
@@ -1,1 @@
> -/* This Source Code Form is subject to the terms of the Mozilla Public

I know I said before that removing this file was fine with me, but I'm having second thoughts here.

We just dropped a hidden preference and told everybody who was complaining to install an add-on using the new NewTabURL.jsm API. And now we are thinking about removing/replacing it only a couple months later.

I think we need to either:
- ensure there's good outreach to add-on authors, and that add-ons using the NewTabURL.jsm API with non trivial numbers of users are updated in time.
- or we need to keep newTabURL.jsm and make it forward calls to the new XPCOM component, probably with a deprecation warning (Deprecated.jsm).

Comment 75

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

(In reply to Florian Quèze [:florian] [:flo] from comment #74)
> We just dropped a hidden preference and told everybody who was complaining
> to install an add-on using the new NewTabURL.jsm API. And now we are
> thinking about removing/replacing it only a couple months later.
> 
> I think we need to either:
> - ensure there's good outreach to add-on authors, and that add-ons using the
> NewTabURL.jsm API with non trivial numbers of users are updated in time.
> - or we need to keep newTabURL.jsm and make it forward calls to the new
> XPCOM component, probably with a deprecation warning (Deprecated.jsm).

I think we need to have a clear path forward here. Florian, which solution would you prefer? Should we consult Jorge?

Comment 76

[Florian Quèze [:florian]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #75)

> I think we need to have a clear path forward here. Florian, which solution
> would you prefer? Should we consult Jorge?

I don't have a strong preference, consulting Jorge is a good idea.

Comment 77

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #75)
> (In reply to Florian Quèze [:florian] [:flo] from comment #74)
> > We just dropped a hidden preference and told everybody who was complaining
> > to install an add-on using the new NewTabURL.jsm API. And now we are
> > thinking about removing/replacing it only a couple months later.
> > 
> > I think we need to either:
> > - ensure there's good outreach to add-on authors, and that add-ons using the
> > NewTabURL.jsm API with non trivial numbers of users are updated in time.
> > - or we need to keep newTabURL.jsm and make it forward calls to the new
> > XPCOM component, probably with a deprecation warning (Deprecated.jsm).

Jorge, in this bug we would like to replace NewTabURL.jsm with an XPCOM component. Now Florian told me that we (more or less) recently told addon developers to use NewTabURL.jsm. Can we reach out to those addon authors to update their code? Or do we in fact have to forward the call under the hood and add a deprecation warning. I would rather like to remove NewTabURL.jsm rather than having a duplicate implementation of the same thing in different files. Jorge, how do you feel about removing NewTabURL.jsm?

Comment 78

[Jorge Villalobos [:jorgev] (he/him)]

I think we should keep the JSM with a deprecation warning, since we already broke this once for everyone. There shouldn't be duplicate implementations because the JSM just wraps the XPCOM component, right?

Comment 79

[Bobby Holley (:bholley)]

As long as the JSM doesn't get loaded (and cost memory) in a normal browser run.

Comment 80

[Marcos Caceres [:marcosc]]

Comment on attachment 8673390 [details] [diff] [review]
bug_1204983_remote_about_newtab_deprecate_newtaburl.patch

Review of attachment 8673390 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM

Comment 81

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: bug_1204983_remote_about_newtab_tests.patch

Rebased the platform tests, carrying over r+ from bholly.

Comment 82

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: bug_1204983_remote_about_newtab.patch

Addressed all the nits by Boris and Bobby, carrying over r+ from them, still need to flag florian for review though.

Comment 83

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Comment on attachment 8674054 [details] [diff] [review]
bug_1204983_remote_about_newtab.patch

@Florian: Addressed all your concerns, this should do it now.

@Dragana: Just the bits within netwerk/base/ so I can resolve that linker problem.

Comment 84

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: bug_1204983_forward_newtaburl_to_aboutnewtabservice.patch

Florian, Olivier I think we can now safely remove "browser.newtabpage.remote" from Bug 1210936, right?

In this patch we just forward all the calls from NewTabURL to aboutNewTabService (including a deprecation warning). Further, also adding a test for aboutNewTabService.

Comment 85

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Comment on attachment 8674056 [details] [diff] [review]
bug_1204983_forward_newtaburl_to_aboutnewtabservice.patch

Review of attachment 8674056 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/newtab/NewTabURL.jsm
@@ +21,2 @@
>    get: function() {
> +    Deprecated.warning("NewTabURL.get is deprecated, please use aboutNewTabService.newTabURL");

Rrrh, didn't fetch my latest changes to the deprecation warning, because it also requires a 'url' as the second argument. So in the end the warnings will look like this:


> Deprecated.warning("NewTabURL.get is deprecated, please use aboutNewTabService.newTabURL", NewTabBugURL);

where NewTabBugURL = "https://bugzilla.mozilla.org/show_bug.cgi?id=1204983";

I will have that updated in the final patch, please disregard for now.

Comment 86

[Florian Quèze [:florian]]

Comment on attachment 8674054 [details] [diff] [review]
bug_1204983_remote_about_newtab.patch

Review of attachment 8674054 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/about/AboutRedirector.cpp
@@ +11,4 @@
>  #include "nsIChannel.h"
>  #include "nsIURI.h"
>  #include "nsIScriptSecurityManager.h"
> +#include "nsIProtocolHandler.h" 

nit: trailing whitespace.

::: browser/components/nsBrowserGlue.js
@@ +2478,5 @@
> +//-------------------------------------
> +
> +function AboutNewTabService()
> +{
> +  this._newtabURL = "about:newtab";

nit: I would prefer this to be _newTabURL rather than _newtabURL, to be consistent with the attribute name.

Comment 87

[Florian Quèze [:florian]]

Comment on attachment 8674056 [details] [diff] [review]
bug_1204983_forward_newtaburl_to_aboutnewtabservice.patch

Review of attachment 8674056 [details] [diff] [review]:
-----------------------------------------------------------------

The changes here look good, but I'm a bit confused as I expected this patch to be an updated version of attachment 8673390 [details] [diff] [review]. We don't want the browser itself to trigger deprecation warnings when no add-on is installed.

::: browser/components/newtab/NewTabURL.jsm
@@ +21,2 @@
>    get: function() {
> +    Deprecated.warning("NewTabURL.get is deprecated, please use aboutNewTabService.newTabURL");

I would prefer the url to point to a page on MDN, but I can't find any page there about NewTabURL.jsm so... I think we should at least make the link point to a specific comment in the bug, and that comment should be short, and explain only what add-on authors need to know to update their add-on.

@@ +29,4 @@
>    },
>  
>    override: function(newURL) {
> +    Deprecated.warning("NewTabURL.override is deprecated, please use aboutNewTabService.newTabURL");

nit: Maybe "please set" instead of "please use" to be explicit that this is a setter.

Comment 88

[Dragana Damjanovic [:dragana]]

Comment on attachment 8674054 [details] [diff] [review]
bug_1204983_remote_about_newtab.patch

Review of attachment 8674054 [details] [diff] [review]:
-----------------------------------------------------------------

I only reviewed netwerk/base/

::: netwerk/base/nsNetUtil.inl
@@ +89,5 @@
>  }
>  
> +nsresult
> +NS_URIChainHasFlags(nsIURI   *uri,
> +                    uint32_t  flags,

add INLINE_IF_EXTERN  in front.

Comment 89

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

***** Attention Addon developers *****

NewTabURL.jsm is deprecated, please use the newly added AboutNewTabService from now on. Here is list of how the old functionality can be updated to use the new one:

* NewTabURL.get()      <==> aboutNewTabService.newTabURL;
* NewTabURL.overridden <==> aboutNewTabService.overridden;
* NewTabURL.override   <==> aboutNewTabService.newTabURL = "...";
* NewTabURL.reset      <==> aboutNewTabService.resetNewTabURL();

Comment 90

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: bug_1204983_remote_about_newtab.patch

Thanks Florian, I addressed all your nits and updated the patch accordignly. Carrying over r+.

Comment 91

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: bug_1204983_remote_about_newtab_deprecate_newtaburl.patch

Florian, you are absolutely right, we have to update all of our code to use the newly added aboutNewTabService and forward all the calls of NewTabURL.jsm to use the new functionality. I addressed all of your concerns, but just to make sure, can you have one more look at this final patch?

Comment 92

[Olivier Yiptong [:oyiptong]]

Comment on attachment 8674424 [details] [diff] [review]
bug_1204983_remote_about_newtab_deprecate_newtaburl.patch

>-// activates the remote-hosted newtab page
>-pref("browser.newtabpage.remote", false);

We want to keep this.


>diff --git a/browser/components/newtab/NewTabURL.jsm b/browser/components/newtab/NewTabURL.jsm
>--- a/browser/components/newtab/NewTabURL.jsm
>+++ b/browser/components/newtab/NewTabURL.jsm
>@@ -5,39 +5,39 @@
> "use strict";
> 
> var Cc = Components.classes;
> var Ci = Components.interfaces;
> var Cu = Components.utils;
> 
> this.EXPORTED_SYMBOLS = [ "NewTabURL" ];
> 
>-Components.utils.import("resource://gre/modules/Services.jsm");
>+Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>+XPCOMUtils.defineLazyServiceGetter(this, "aboutNewTabService",
>+                                   "@mozilla.org/browser/aboutnewtab-service;1",
>+                                   "nsIAboutNewTabService");
>+XPCOMUtils.defineLazyModuleGetter(this, "Deprecated",
>+                                  "resource://gre/modules/Deprecated.jsm");
>+
>+const DepecationURL = "https://bugzilla.mozilla.org/show_bug.cgi?id=1204983#c89";
> 
> this.NewTabURL = {
>-  _url: "about:newtab",
>-  _remoteUrl: "about:remote-newtab",
>-  _overridden: false,
> 
>   get: function() {
>-    let output = this._url;
>-    if (Services.prefs.getBoolPref("browser.newtabpage.remote")) {
>-      output = this._remoteUrl;
>-    }
>-    return output;

There is some logic there to return an alternate NewTabURL if the boolean pref is true.
This logic should be relocated in AboutNewTabService in nsBrowserGlue.js.

Comment 93

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: bug_1204983_remote_about_newtab_deprecate_newtaburl.patch

Oliver, good that I flagged you for review. Didn't know we wanted to keep the pref: "browser.newtabpage.remote". I put it back and updated the tests (which still pass :-).

Let me just double check with bholly real quick to make sure everything is still fine with that update.

Comment 94

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #93)
> Created attachment 8674473 [details] [diff] [review]
> bug_1204983_remote_about_newtab_deprecate_newtaburl.patch
> 
> Oliver, good that I flagged you for review. Didn't know we wanted to keep
> the pref: "browser.newtabpage.remote". I put it back and updated the tests
> (which still pass :-).
> 
> Let me just double check with bholly real quick to make sure everything is
> still fine with that update.

Hey Bobby, I suppose it's fine but wanted to make sure that you see it the same way. Olivier just told me they would like to keep the pref: browser.newtabpage.remote. So we decided to just move the pref over from newTabURL.jsm into aboutNewTabService (within nsBrowserGlue.js). It shouldn't affect any of our changes within AboutRedirector, because setting the LOAD_REPLACE flag happens after the fact. Just wanted to check if there are any other side effects this might cause.

Comment 95

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Comment on attachment 8674473 [details] [diff] [review]
bug_1204983_remote_about_newtab_deprecate_newtaburl.patch

Would still be great to get Florian's green lights on that patch as well.

Comment 96

[Florian Quèze [:florian]]

Comment on attachment 8674473 [details] [diff] [review]
bug_1204983_remote_about_newtab_deprecate_newtaburl.patch

Review of attachment 8674473 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. Please verify with the try server that this doesn't break the browser mochitests or xpcshell tests before landing.

::: browser/components/nsBrowserGlue.js
@@ +2489,5 @@
>    QueryInterface: XPCOMUtils.generateQI([Ci.nsIAboutNewTabService]),
>  
>    get newTabURL() {
> +    if (Services.prefs.getBoolPref("browser.newtabpage.remote")) {
> +      return "about:remote-newtab";

Shouldn't add-ons also be able to override this? (ie. add an "&& !this._overriden" to the test here)

Comment 97

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

(In reply to Florian Quèze [:florian] [:flo] from comment #96)
> >    get newTabURL() {
> > +    if (Services.prefs.getBoolPref("browser.newtabpage.remote")) {
> > +      return "about:remote-newtab";
> 
> Shouldn't add-ons also be able to override this? (ie. add an "&&
> !this._overriden" to the test here)

I think that's a question for Olivier or Marcos. What do you think? How would you like to have that handled?

Comment 98

[Bobby Holley (:bholley)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #94)
> Hey Bobby, I suppose it's fine but wanted to make sure that you see it the
> same way. Olivier just told me they would like to keep the pref:
> browser.newtabpage.remote. So we decided to just move the pref over from
> newTabURL.jsm into aboutNewTabService (within nsBrowserGlue.js). It
> shouldn't affect any of our changes within AboutRedirector, because setting
> the LOAD_REPLACE flag happens after the fact. Just wanted to check if there
> are any other side effects this might cause.

This doesn't make sense to me. What is the semantic meaning of about:newtab-remote? Is that considered to be 'overridden'?

Given that we're doing the LOAD_REPLACE thing, the docshell url is never going to be about:newtab-remote - it's either going to be 'about:newtab" in the local case, or 'http://foo' in the remote case. So I don't understand what value it adds, and how it would possibly do the right thing given the machinery in the patch.

Comment 99

[Olivier Yiptong [:oyiptong]]

(In reply to Bobby Holley (:bholley) from comment #98)
> (In reply to Christoph Kerschbaumer [:ckerschb] from comment #94)
> > Hey Bobby, I suppose it's fine but wanted to make sure that you see it the
> > same way. Olivier just told me they would like to keep the pref:
> > browser.newtabpage.remote. So we decided to just move the pref over from
> > newTabURL.jsm into aboutNewTabService (within nsBrowserGlue.js). It
> > shouldn't affect any of our changes within AboutRedirector, because setting
> > the LOAD_REPLACE flag happens after the fact. Just wanted to check if there
> > are any other side effects this might cause.
> 
> This doesn't make sense to me. What is the semantic meaning of
> about:newtab-remote? Is that considered to be 'overridden'?

about:remote-newtab is a temporary url we introduced so that we could run both newtabs side-by-side.
The pref allows about:remote-newtab to become the default newtab page URL.

In that sense, I would say it wouldn't be considered 'overridden'.

> 
> Given that we're doing the LOAD_REPLACE thing, the docshell url is never
> going to be about:newtab-remote - it's either going to be 'about:newtab" in
> the local case, or 'http://foo' in the remote case. So I don't understand
> what value it adds, and how it would possibly do the right thing given the
> machinery in the patch.

LOAD_REPLACE allows us to not need about:remote-newtab. However, until the rest of our code is ready, we can't make the switch. We have code that currently communicates to a remote page inside an iframe in a  content frame.

With Christoph's patch, we'll be able to do the switch and simplify our code.

Comment 100

[Bobby Holley (:bholley)]

I see. So this basically doesn't affect the about:newtab case, (since overridden will be false when we query it from AboutRedirector).

I presume we have a blocking bug on file for removing about:remote-newtab before shipping? If it's just an interim thing then it's fine.

Comment 101

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Bobby, can we live with that pref temporarily? Hopefully it shouldn't live that much longer.

Comment 102

[Olivier Yiptong [:oyiptong]]

Comment on attachment 8674473 [details] [diff] [review]
bug_1204983_remote_about_newtab_deprecate_newtaburl.patch

Review of attachment 8674473 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with one small change

::: browser/components/nsBrowserGlue.js
@@ +2488,5 @@
>    classID: Components.ID("{97eea4bb-db50-4ae0-9147-1e5ed55b4ed5}"),
>    QueryInterface: XPCOMUtils.generateQI([Ci.nsIAboutNewTabService]),
>  
>    get newTabURL() {
> +    if (Services.prefs.getBoolPref("browser.newtabpage.remote")) {

this check should also happen in resetNewTabURL

Comment 103

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

(In reply to Olivier Yiptong [:oyiptong] from comment #102)
> >    get newTabURL() {
> > +    if (Services.prefs.getBoolPref("browser.newtabpage.remote")) {
> 
> this check should also happen in resetNewTabURL

As discussed on IRC, we are just replicating the functionality of NewTabURL.jsm within aboutNewTabService. Since it's currently not the case within NewTabURL.jsm I am also not going to update that within aboutNewTabService.

Comment 104

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Marcos, Olivier answered all of my questions - clearing needinfo.

Comment 105

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Tests pass locally - let's hope that TRY feels the same way:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=eaafed667b36

Comment 106

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #105)
> Tests pass locally - let's hope that TRY feels the same way:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=eaafed667b36

Oh, interesting error:
> mismatch in conditional expression: 'nsIRequest::<anonymous enum>' vs
> 'nsIChannel::<anonymous enum>' [-Werror=enum-compare]

static_cast to the rescue :-) here is a new TRY run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=681c292d2548

As far as I can tell all these errors are intermittents, I'll try to land those patches.

Comment 107

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c7d3635f771a
https://hg.mozilla.org/integration/mozilla-inbound/rev/602e91d48e43
https://hg.mozilla.org/integration/mozilla-inbound/rev/5bdf50e82ca8

Comment 108

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

All backed out for various talos failures like https://treeherder.mozilla.org/logviewer.html#?job_id=15760108&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/69af7cab1bc0

Comment 109

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

(In reply to Wes Kocher (:KWierso) from comment #108)
> All backed out for various talos failures like
> https://treeherder.mozilla.org/logviewer.html#?job_id=15760108&repo=mozilla-
> inbound
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/69af7cab1bc0

I have never seen such Talos errors before, Florian, Bobby, any idea what might go wrong here?

Comment 110

[Bobby Holley (:bholley)]

From the log, it appears that you're hitting a timeout when

/builds/slave/test/build/application/firefox/firefox -profile /tmp/tmp2zjXz4/profile -tp file:/builds/slave/test/build/tests/talos/talos/tests/tabswitch/tps.manifest.develop -tpchrome -tpnoisy -tploadnocache -tpcycles 1 -tppagecycles 5

is run, during "tps" (whatever that is).

Joel Maher is a good person to ask about Talos in general, but I suspect you're going to need to reproduce this and debug what's going on.

Comment 111

[Joel Maher ( :jmaher ) (UTC -8)]

oh, we need to figure this out.  Can you run talos locally and reproduce this:
https://wiki.mozilla.org/Buildbot/Talos/Running#Running_locally_-_Source_Code

I would run:
talos -a tps --debug -e full/path/to/firefox


I see tart failing as well. most like this is probably going to require us to change our use of overriding about:blank new tab in talos:
https://dxr.mozilla.org/mozilla-central/search?q=path%3Atesting%2Ftalos+override%28%22about&redirect=false&case=true

Comment 112

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Thanks Joel for you help in setting up Talos. I finally found the root cause of the problem we are facing here. It's the tests
* newtabNoPreload, and 
* newtabYesPreload
which both set
> aboutNewTabService.newTabURL = "about:newtab"
which in the end turns out to be and endless loop :-(

We have two options, either:
* we can remove those two tests
* or (my preferred solution) we add a check in aboutRedirector to make sure that newtab can not redirect to about:newtab, so basically something like:
> if (url.EqualsLiteral("about:newtab")) {
>   url.EqualsLiteral("newtab");
> }

In my opinion it makes sense to have that check, because it's a fairly common mistake to call aboutNewTabService.newTabURL = "about:newtab".

In fact, I even think we have that problem if aboutNewTabService.newTabURL never gets a value assigned, because per default it returns "about:newtab" which would run into the same problem.

Bobby, what do you think?

Comment 113

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #112)
> that newtab can not redirect to about:newtab, so basically something like:
> > if (url.EqualsLiteral("about:newtab")) {
> >   url.EqualsLiteral("newtab");
> > }

I mean default to what's currently set in the map obviously.

> In my opinion it makes sense to have that check, because it's a fairly
> common mistake to call aboutNewTabService.newTabURL = "about:newtab".
> 
> In fact, I even think we have that problem if aboutNewTabService.newTabURL
> never gets a value assigned, because per default it returns "about:newtab"
> which would run into the same problem.

Ah, the reason we don't have that problem is because .overridden would still be false and hence it defaults to what's in the map.

Comment 114

[Bobby Holley (:bholley)]

Yeah, can't we just make the newTabURL treat an assignment to about:newtab equivalently with a reset?

Comment 115

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: bug_1204983_remote_about_newtab_update.patch

Sounds very reasonable to me, let's just call resetNewTabURL in that case. Talos tests work now.

Comment 116

[Bobby Holley (:bholley)]

Comment on attachment 8675860 [details] [diff] [review]
bug_1204983_remote_about_newtab_update.patch

Review of attachment 8675860 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/nsBrowserGlue.js
@@ +2495,5 @@
>      return this._newTabURL;
>    },
>  
>    set newTabURL(aNewTabURL) {
> +    if (aNewTabURL === "about:newtab") {

Can you define this string as a constant in this file so that we only use it once? r=me with that.

Comment 117

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: bug_1204983_remote_about_newtab.patch

Incorporated Bobby's suggestion and qfolded the two patches into one. Carrying over r+ from bholley.

Comment 118

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: bug_1204983_remote_about_newtab_deprecate_newtaburl.patch

Just rebased that patch on top of the other patches. Carrying over r+.

Comment 119

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/38b6f878608a
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4879b180d2d
https://hg.mozilla.org/integration/mozilla-inbound/rev/00c07b02c8a8

Comment 120

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

This appears to have made talos(g2) permafail: https://treeherder.mozilla.org/logviewer.html#?job_id=15886460&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/1e2fc068682e

Comment 121

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

(In reply to Wes Kocher (:KWierso) from comment #120)
> This appears to have made talos(g2) permafail:
> https://treeherder.mozilla.org/logviewer.html#?job_id=15886460&repo=mozilla-
> inbound
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/1e2fc068682e

Oh nice - rrrh!

Comment 122

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #121)
> (In reply to Wes Kocher (:KWierso) from comment #120)
> > This appears to have made talos(g2) permafail:
> > https://treeherder.mozilla.org/logviewer.html#?job_id=15886460&repo=mozilla-
> > inbound

At least the 'tart' errors are gone! So that's something. Joel, the error is still somehow cryptic to me, can you provide some guidance on how we can move forward here? Is there a possibility I can I run 'g2' only on my local machine? If so, how can I do that? Or anything else you see when looking at the log?

Comment 123

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

So, looking through the logs it seems like a tps error, so I am running:
> talos -a tps --debug -e /home/ckerschb/moz/mc-obj-dbg/dist/bin/firefox --develop

which causes a GC crash (see stack underneath), but that is not the same crash we see on Treeherder. Without the patches applied it runs fine however, so we have to do more debugging I suppose.

(gdb) bt
#0  0x00007fa376e1ef3d in nanosleep () at ../sysdeps/unix/syscall-template.S:81
#1  0x00007fa376e1edd4 in __sleep (seconds=0) at ../sysdeps/unix/sysv/linux/sleep.c:137
#2  0x00007fa3693e16d6 in ah_crap_handler (signum=11) at /home/ckerschb/moz/mc/toolkit/xre/nsSigHandlers.cpp:103
#3  0x00007fa3693bf9d0 in nsProfileLock::FatalSignalHandler (signo=11, info=0x7ffc519d42b0, context=0x7ffc519d4180) at /home/ckerschb/moz/mc/toolkit/profile/nsProfileLock.cpp:195
#4  0x00007fa36af8123f in AsmJSFaultHandler (signum=11, info=0x7ffc519d42b0, context=0x7ffc519d4180) at /home/ckerschb/moz/mc/js/src/asmjs/AsmJSSignalHandlers.cpp:1158
#5  <signal handler called>
#6  mozilla::LinkedList<js::UnboxedLayout>::~LinkedList (this=0x7fa359aa69b8) at ../../dist/include/mozilla/LinkedList.h:328
#7  0x00007fa36a8504a6 in JSCompartment::~JSCompartment (this=0x7fa359aa6600) at /home/ckerschb/moz/mc/js/src/jscompartment.cpp:112
#8  0x00007fa36a8983f3 in js_delete<JSCompartment> (p=0x7fa359aa6600) at ../../dist/include/js/Utility.h:370
#9  0x00007fa36a8a0f1b in JS::Zone::sweepCompartments (this=0x7fa35fbd8a00, fop=0x7ffc519d4a40, keepAtleastOne=false, destroyingRuntime=true)
    at /home/ckerschb/moz/mc/js/src/jsgc.cpp:3649
#10 0x00007fa36a8a1384 in js::gc::GCRuntime::sweepZones (this=0x7fa35fbb2410, fop=0x7ffc519d4a40, destroyingRuntime=true) at /home/ckerschb/moz/mc/js/src/jsgc.cpp:3690
#11 0x00007fa36a8a8563 in js::gc::GCRuntime::endSweepPhase (this=0x7fa35fbb2410, destroyingRuntime=true) at /home/ckerschb/moz/mc/js/src/jsgc.cpp:5507
#12 0x00007fa36a8aa6f0 in js::gc::GCRuntime::incrementalCollectSlice (this=0x7fa35fbb2410, budget=..., reason=JS::gcreason::DESTROY_RUNTIME)
    at /home/ckerschb/moz/mc/js/src/jsgc.cpp:6013
#13 0x00007fa36a8ab251 in js::gc::GCRuntime::gcCycle (this=0x7fa35fbb2410, nonincrementalByAPI=true, budget=..., reason=JS::gcreason::DESTROY_RUNTIME)
    at /home/ckerschb/moz/mc/js/src/jsgc.cpp:6222
#14 0x00007fa36a8aba5b in js::gc::GCRuntime::collect (this=0x7fa35fbb2410, nonincrementalByAPI=true, budget=..., reason=JS::gcreason::DESTROY_RUNTIME)
    at /home/ckerschb/moz/mc/js/src/jsgc.cpp:6338
#15 0x00007fa36a89ef66 in js::gc::GCRuntime::gc (this=0x7fa35fbb2410, gckind=GC_NORMAL, reason=JS::gcreason::DESTROY_RUNTIME) at /home/ckerschb/moz/mc/js/src/jsgc.cpp:6402

Comment 124

[Joel Maher ( :jmaher ) (UTC -8)]

it looks like a hang in production.  When you run it locally, does the crash happen immediately?  It does look as though you are running a debug version locally which might account for the difference.

Comment 125

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: bug_1204983_remote_about_newtab_update_content_test.patch

I created an optimized build and locally ran
> talos -a tps --debug -e /home/ckerschb/moz/mc-obj-opt/dist/bin/firefox --develop
which then times out eventually with no obvious error. At least no prints on the console.

I started to take out different bits of our patches to narrow down the problem and I suddenly realized that the bits within 'testing/talos/talos/tests/tabswitch/content/test.html' cause the problem. Don't ask me why. If you know why, please let me know Bobby.

Let's run all the Talos tests on Treeherder again and see if that really was the problem:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=76bd3264e460

Comment 126

[Bobby Holley (:bholley)]

Comment on attachment 8676549 [details] [diff] [review]
bug_1204983_remote_about_newtab_update_content_test.patch

Review of attachment 8676549 [details] [diff] [review]:
-----------------------------------------------------------------

I think we need to understand what's going on here.

Looks like you're hitting this:

http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/XPCJSID.cpp#687

Debug and figure out why getService call is failing?

Comment 127

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

(In reply to Bobby Holley (:bholley) from comment #126)
> Comment on attachment 8676549 [details] [diff] [review]
> bug_1204983_remote_about_newtab_update_content_test.patch
> 
> Review of attachment 8676549 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think we need to understand what's going on here.
> 
> Looks like you're hitting this:
> 
> http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/XPCJSID.
> cpp#687
> 
> Debug and figure out why getService call is failing?

Sorry, I should have mentioned that when I flagged you for review. The reason I was hitting the 'getService' problem is because I started to split the patches and when running Talos tests the applied patches were missing the AboutNewTabService as an component:
> var components = [BrowserGlue, ContentPermissionPrompt];
> this.NSGetFactory = XPCOMUtils.generateNSGetFactory(components);
and hence we hit that error. Sorry for the confusion.

Let's wait for the try run to finish though before we move on here. Unless you have a gut feeling why this new patch could fix things.

Comment 128

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Comment on attachment 8676549 [details] [diff] [review]
bug_1204983_remote_about_newtab_update_content_test.patch

It seems to have fixed the problem:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=76bd3264e460

I can't reason why though. What do you think?

Comment 129

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: bug_1204983_remote_about_newtab_deprecate_newtaburl.patch

Thanks Bobby; just qfolding the two patches into one; carrying over r+ from bholley.

Comment 130

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8694c40c0a2a
https://hg.mozilla.org/integration/mozilla-inbound/rev/11e0112b3dec
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf0ce9ac5997

Comment 131

[Joel Maher ( :jmaher ) (UTC -8)]

please ignore the linux64 tresize regression email, it is a false alarm.

Comment 132

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8694c40c0a2a
https://hg.mozilla.org/mozilla-central/rev/11e0112b3dec
https://hg.mozilla.org/mozilla-central/rev/bf0ce9ac5997

Comment 133

[Mike Kaply [:mkaply]]

Why even have the deprecation warning?

Wouldn't it make more sense to just keep NewTabURL.jsm as the normal way to interface, that way if something changes again, you can just change it in the backend?

There's no advantage to using the new service, is there?

Don't we want to encourage people to use the JSM instead of the service?

Comment 134

[Ben Bucksch (:BenB)]

I concur with Mike Kaply.

Comment 135

[Mike Kaply [:mkaply]]

Can someone please respond to comment 133?

Why are we forcing people to use a service when a JavaScript module can be used?

Comment 136

[sblask]

I thought XPCOM is about to get deprecated (https://blog.mozilla.org/addons/2015/08/21/the-future-of-developing-firefox-add-ons/) isn't this change a move into the wrong direction? I don't want to change my add-on just to have to change it back or to something else just in a few more months time.

Comment 137

[Ben Basson]

Speaking as an add-on developer of many years, I have to say it seems like you guys don't have much of a plan any more. 

NewTabURL.jsm has hardly been around for a long time and now you're already telling us to move off of it - towards something that has less wrapping code and therefore could break add-ons when changed? Does anyone actually care about add-ons any more?

Comment 138

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

I understand the concerns of add-on developers, but before causing any more confusion, I let Oliver respond.

Comment 139

[Olivier Yiptong [:oyiptong]]

When NewTabURL.jsm was landed, there was no plan to make the newtab page remote yet (bug 1118285).

Now, we have 2 requirements relevant to this bug:

1) load unprivileged remotely hosted pages in about:newtab
2) keep the newtab url overridable as in NewTabURL.jsm

We are accomplishing both with the service.

In response to comment 133, the service was made partly because we need to interface with aboutRedirector.cpp to accomplish #1.

The service still allows overriding the url using an addon. We could keep NewTabURL.jsm around (which still functions, but is Deprecated), but what advantage to we have in keeping it if the service is easily overridable?

Comment 140

[Ben Bucksch (:BenB)]

> We are accomplishing both with the service.

And both of that you can also accomplish with the existing NewTabURL.jsm. Why don't you just change the implementation of that JSM?

> what advantage to we have in keeping [the JSM]?

Not breaking a public API that you just recently introduced. If you create an API and ask *all* extensions that use it to update, then the new API should better last for the foreseeable future.

Our problem is that the new API is less generic - and thus more prone to be changed *again* - than the current JSM one. You can do everything with the JSM, no matter how things are implemented, but you cannot with the service.

Even the JSM was not generic enough, because it was only about newtab. It should have been one API for all "controlled" prefs, including search and others. Now you make a new one which is even more specific to your current implementation. That's where we're getting the impression that there's no plan for proper APIs.

Comment 141

[Ben Bucksch (:BenB)]

Basically, we're saying: Do not depreacte NewTabURL.jsm. You've just recently introduced it, and asked us to change to it, and we did, now please support it (horizon for any new APIs should be 5-10 years, not days).

Comment 142

[Bobby Holley (:bholley)]

(In reply to Ben Bucksch (:BenB) from comment #141)
> Basically, we're saying: Do not depreacte NewTabURL.jsm. You've just
> recently introduced it, and asked us to change to it, and we did, now please
> support it (horizon for any new APIs should be 5-10 years, not days).

I'm not aware of any stability guarantees that anybody has made about JSM APIs (feel free to point me to them if I've missed that somewhere). This is why maintaining an XPCOM addon is hard, and why we are encouraging people to use WebExtensions, which is an actual addon API with a support plan.

Comment 143

[Mike Kaply [:mkaply]]

The switch to the new interface is encouraging people to use XPCOM.

At least with the JSM, what happened on the backend didn't matter.

That's the point we're making here. The JSM works and works fine. Why are you encouraging people to switch to an XPCOM interface at this point?

Comment 144

[Bobby Holley (:bholley)]

(In reply to Mike Kaply [:mkaply] from comment #143)
> The switch to the new interface is encouraging people to use XPCOM.
> 
> At least with the JSM, what happened on the backend didn't matter.

When I say "XPCOM addons", I am referring to all the stuff that operates directly with browser internals, including XPCOM components and JSMs. There is no difference in stability guarantees between XPCOM components and JSMs, because there are no guarantees for either.

> That's the point we're making here. The JSM works and works fine.

For your purposes yes. But not for the purposes that motivated this change. We now have C++ code that needs to ask about the NewTab URL, and it's easier to talk to a component from C++ than to a JSM.

I don't know/remember enough about the details here to determine whether there was some way to have this information be available both in the new way and the old way without introducing unacceptable complexity. That is a question to take up with Olivier.

Comment 145

[Ben Basson]

(In reply to Bobby Holley (busy) from comment #142)
> I'm not aware of any stability guarantees that anybody has made about JSM
> APIs (feel free to point me to them if I've missed that somewhere). This is
> why maintaining an XPCOM addon is hard, and why we are encouraging people to
> use WebExtensions, which is an actual addon API with a support plan.

According to the WebExtensions wiki page, it's not possible to publish WebExtensions on AMO yet, so excuse me if I don't jump right to it, assuming it even offers the same functionality (which for most of my add-ons, it doesn't).

FYI - the entire reason that NewTabURL.jsm was implemented in the first place was as a hook for add-ons. See bug 1118285.

Comment 146

[Ben Bucksch (:BenB)]

Bobby, please point me to the stable WebExtensions API for NewTab, that I can use in production today. We have an extension with a few million customers.

> For your purposes yes. But not for the purposes that motivated this change.

NewTabURL.jsm can implement this other API, and in fact it already does. All we're asking is not to deprecate it. There's no reason to deprecate it.

Comment 147

[Olivier Yiptong [:oyiptong]]

(In reply to Ben Basson from comment #145) 
> FYI - the entire reason that NewTabURL.jsm was implemented in the first
> place was as a hook for add-ons. See bug 1118285.

IUUC, the reason NewTabURL.jsm was implemented was to diminish the risk of hijacking, while still allowing addons to override.

(In reply to Ben Bucksch (:BenB) from comment #146)
> NewTabURL.jsm can implement this other API, and in fact it already does. All
> we're asking is not to deprecate it. There's no reason to deprecate it.

The service is a more capable, if a little more complicated piece of code. One should use it rather than NewTabURL.jsm at this point. One of the reasons why we want to deprecate it is because it's just more code to maintain. At this point, it is just a simpler interface to aboutNewTabService.

However, I propose this:

1) Keep NewTabURL.jsm in deprecated state for the forseeable future (6 months? a year?). It will be kept in a functional state throughout changes in aboutNewTabService. It is deprecated in the sense that no new features will be added
2) Perhaps some documentation/explanation as to how aboutNewTabService works and what it is capable of doing

It would be useful to know what your expectations are with respect to how you're using NewTabURL.jsm / aboutNewTabService.

Comment 148

[sblask]

My understanding as add-on developer is that require('chrome') should no be used anymore as it's not compatible with multi-procss Firefox(From https://blog.mozilla.org/addons/2015/08/21/the-future-of-developing-firefox-add-ons/: "Add-ons based on the Jetpack SDK will work well as long as they don’t use require(‘chrome’) or some of the low-level APIs to touch objects in the content process.") Using the service does require using require('chrome') which also affects reviewing as far as I understand.

Apart from that there is the pending deprecation of XPCOM. I would like to keep NewTabURL.jsm around as long as there is no API with a long-term plan to replace it.

Comment 149

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

> (In reply to Ben Bucksch (:BenB) from comment #146)
> > NewTabURL.jsm can implement this other API, and in fact it already does. All
> > we're asking is not to deprecate it. There's no reason to deprecate it.

After chatting with a variety of people and thanks to the input of everyone on this bug we decided to remove the deprecation warning from NewTabUrl.jsm and keep the file around, see Bug 1239085.

Comment 150

[[e-mail]]

(In reply to Christoph Kerschbaumer from comment #89)
> ***** Attention Addon developers *****
> 
> NewTabURL.jsm is deprecated, please use the newly added AboutNewTabService
> from now on.

Looks like this didn't make https://developer.mozilla.org/en-US/Firefox/Releases/44 :-(

Comment 151

[Mike Kaply [:mkaply]]

It's worse than that. NewTabURL.jsm doesn't appear to be in Firefox 44 at all.

resource:///modules/NewTabURL.jsm

File not found.

Did I miss something?

Comment 152

[Olivier Yiptong [:oyiptong]]

No, as per bug 1240559, it's too late to uplift for 44.

Can you use the service for now?

Comment 153

[Mike Kaply [:mkaply]]

We have partner builds that had code for this in. So those will be broken out of the box.

And no, we can't get new code written and new builds tested in 1 day (nothing over the weekend).

Why on earth wasn't this tested and why wasn't the problem communicated to add-on developers?

Comment 154

[Olivier Yiptong [:oyiptong]]

Sorry, it's my fault. I didn't realize it would've impacted add-on developers and we're only discovering this now.

NewTabURL.jsm was excluded at build time in bug 1224950. This was a security measure.
This has been fixed in 46, in bug 1218996.

We haven't noticed the issue, because we use the service internally.

Comment 155

[Mike Conley (:mconley) (:⚙️)]

For the folks in this bug who were affected by the missing NewTabURL.jsm, a patch to put it back was landed in bug 1240559 and should work in 44.

Comment 156

[Aris]

Sorry to post about new tab page overriding after the bug was closed, but new tab overriding still seems not to work correctly.

F43 & overriding new tab page using NewTabURL.jsm:
new tab button/tab: loads the url new tab page gets overridden with
typing 'about:newtab' into address bar: loads 'about:newtab'

F44/45beta & overriding new tab page using NewTabURL.jsm or NewTab service:
new tab button/tab: loads the url new tab page gets overridden with
typing 'about:newtab' into address bar: --> ERROR page

F46 & overriding new tab page using NewTabURL.jsm or NewTab service
new tab button/tab: --> ERROR page
typing 'about:newtab' into address bar: --> ERROR page

F47 & overriding new tab page using NewTabURL.jsm or NewTab service:
new tab button/tab: loads the url new tab page gets overridden with
typing 'about:newtab' into address bar: loads 'about:newtab'


So basically only Firefox 47 Nightly works like Firefox 43 did. Everything in between causes errors at least in one case.

Can somebody verify/check this?
And shouldn't typing 'about:newtab' into address bar point to the page we override new tab page with?

Comment 157

[Olivier Yiptong [:oyiptong]]

Aris, can you check if this is related to: bug 1240169?

If so, release(44), beta (45) and nightly (47) should work. For now, only aurora (46) should be affected, but a fix is in the works.

If you're overriding the newtab page, you probably care about opening the page with the "+" button or via shortcut key.

Also, can you please send me a link to your addon? I could take a look.

Comment 158

[Aris]

Ok, I understand typing 'about:newtab' into address bar is an own case and will and should point to the new tab page regardless of any overriding and that is fine. So, only the bug remains, where this case leads to the error / not found page (in Fx 44 and 45), right?

By the way the issue is independent of my add-on, because I can reproduce it on a new profile using "New Tab Override" add-on too: https://addons.mozilla.org/addon/new-tab-override/

CTR: https://addons.mozilla.org/addon/classicthemerestorer/
Direct link: https://addons.mozilla.org/firefox/downloads/file/393215/classic_theme_restorer-1.4.7-fx.xpi

CTR preferences -> Tabs 2 category
-> check 'Custom new tab page' checkbox
-> type custom url into textbox

Code:
xpi file /content/overlay.js line 2006 ff
https://addons.mozilla.org/files/browse/393215/file/content/overlay.js#L2006

Using that code breaks 'about:newtab' url in address bar, but works fine for new tab button/tab.